-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document decision tree steps in report #959
Conversation
@handwerkerd do you know why the paths to the artifacts in the CircleCI config were wrong? Is it just that we changed the actual output folders in the DTM PR and forgot to update the artifact paths, or did we choose to stop uploading artifacts because of free plan limits or something similar? |
I didn't get through all of the Kundu decision tree steps, because some are quite complicated, so the report will be incomplete for that one. |
Looking at your change, I think that's what happened. I changed the location of the outputs for testing, but didn't realize I should have also made this change. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
==========================================
- Coverage 89.54% 89.37% -0.18%
==========================================
Files 26 26
Lines 3396 3397 +1
Branches 619 619
==========================================
- Hits 3041 3036 -5
- Misses 207 211 +4
- Partials 148 150 +2 ☔ View full report in Codecov by Sentry. |
I think I've successfully documented the remaining nodes of the kundu tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of comments that I find impossible to understand. Could you please make them more understandable to the regular user/contributor? Thanks!
"tag_if_true": "Unlikely BOLD" | ||
"tag_if_true": "Unlikely BOLD", | ||
"log_extra_info": "", | ||
"log_extra_report": "Any components with negative t-statistic comparing distribution of T2* model F-statistics from voxels in clusters to those of voxels not in clusters and variance explained greater than median are rejected." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not easy to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this?
"log_extra_report": "Any components with negative t-statistic comparing distribution of T2* model F-statistics from voxels in clusters to those of voxels not in clusters and variance explained greater than median are rejected." | |
"log_extra_report": "Any components with (1) greater than median variance explained and (2) a t-statistic less than zero, determined from a t-test of T2* model F-statistics from voxels in clusters (i.e., "signal" voxels) to voxels not in clusters (i.e., "noise" voxels), are rejected." |
"log_extra_info": "", | ||
"log_extra_report": "If any components are still labeled as unclassified or unclassified high-variance, then a revised decision table score is calculated from the provisionally accepted, unclassified, and unclassified high-variance components." | ||
}, | ||
"_comment": "Calculate a new d_table_score. Why can't we use the original d_table_score function? Because it's a metric function?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a comment for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the _comment
s are, I believe, for developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log_extra_report
is for text that will be outputted in the reports. _comment
lives just in this file and can be used to provide additional information to anyone who decides to read the json file. In this case, the _comment should be changed.
"_comment": "Calculate a new d_table_score. Why can't we use the original d_table_score function? Because it's a metric function?" | |
"_comment": "Calculate a new d_table_score. 5 metric are sorted and ranked across components and this is the average. A new score is calculated on a subset of components because the rankings will change depending on the removed components" |
@tsalo I've been told there's a typo in the docs:
Could you please fix it in this PR? Just asking because this is docs-related. |
I don't think they can. Previous papers that I've read that describe the decision tree don't cover everything in sufficient detail. The main paper we cite for the v2.5 decision tree is Kundu et al. (2013), but that paper (and its supplement) seems to only describe Kappa- and Rho-based decisions. Olafsson et al. (2015) describes each of the steps in the decision tree in some detail, but it's completely missing the additional constraint of greater-than-median variance explained in several of the nodes. Plus, that only applies to the v2.5 decision tree. The minimal decision tree doesn't have a paper that describes each step AFAIK, nor would custom decision trees.
I think that is the best way to handle it.
I think all of this information is necessary for manuscripts, but I can also see people moving some contents into their supplementary materials. I've seen folks do this with fMRIPrep methods sections as well. |
When I wrote "someone can look at several places to see what happens in that decision tree." I meant that there are several places in the existing outputs to understand what happens, but I agree these aren't detailed in any previous publication. I'm going to disagree that all this information should be included in a manuscript. I've included the report output that would be generated by this PR for the kundu tree below. Not only is this long, but it's confusing and a poor way for anyone to understand what is happening in this process. We can edit various sentences, but I don't think anyone who reads any block of text like that would leave with a solid understanding of the process. If I reviewed a manuscript with a paragraph like that, I'd ask for revisions. The flow chart is a much better way of sharing and understanding this process. This is an example of the limits of manuscripts as a format and the original purpose for things like BIDS. If a manuscript includes details of every step to this level of detail, it would very long and unreadable. For example, there are many critical MRI acquisition parameters that are never reported in manuscripts, but I worked with the original BIDS group to make sure most of this acquisition parameter list was in the BIDS jsons. I think we should take a similar approach here. We should have very clear output files that make it easy for people to know what happened and should expect those files to be shared with data. Right now the relevant data is in several places. (1) The flow chart (2) The info file Additional things we can do would be to generate a run-specific flow chart (#888) or just pull the Here's what the report text currently looks like:
|
The plan for this was to create a DOI for the decision trees so that the json file could reference the tree and include that in the report. In the last meeting we suggested doing this through zenodo or figshare, but I realized we could also get DOIs from osf.io and that's already where tedana is sharing other data. My plan would be to create a new project That said, one issue I have is that I'd like to store the json file and the flow chart visualization at the DOI link. The trouble is that I'd need to create a DOI and then upload the json file with the newly created DOI into the project. I'm not sure if that newly created DOI would then reference a version of the project before the json was uploaded. Anyone have experience with this? Update: I'm realizing that the DOI would go into Update 2: https://help.osf.io/article/220-create-dois "DOIs point to the current version of the project or registration. OSF does not support DOI versioning at this time." Since we specifically want to generate updated DOIs with content updates, we cannot use OSF. I'm fairly sure both zenodo and figshare will let me create one project with multiple files where I can get a new doi for each version. Anyone of preferences for figshare vs zenodo? |
I feel like FigShare might be a better fit for this, but I don't have a strong preference. |
I have never used FigShare so I have no opinion on this. |
@tsalo Could you try to merge Main into this PR? I tried to do it on my branch and I'm running into problems. If you get it working, that might make it easier for me to figure out what's going wrong on my end. See: #970 (comment) |
@handwerkerd I updated the branch yesterday, but forgot to comment to that effect until now. Sorry about that! |
Planned changes from past developers calls:
|
Making updates we discussed
Aligned with main
Not sure if anyone but @tsalo can see this yet, but I uploaded the decision trees to figureshare in a new This is still private because I think there are limits to what I can change once I publish, but I can probably give individuals access to let me know if it includes what you think it should include. |
The current approach of PRs to my branch are confusing me, so I've changed the target branch to a new one under ME-ICA ( |
This is either my git skills or a permissions issue, but I'm not sure how I can push directly to https://github.com/tsalo/tedana/tree/doc-tree. My current updates are now in https://github.com/handwerkerd/tedana/tree/doc-tree I also fixed the five-echo test failure above, but there's a new error with five-echo and I'm having trouble solving. Five-echo is the one that uses the minimal decision tree with this line for reports: |
@handwerkerd I have "allow edits by maintainers" selected, so you should be able to push to my branch if you add it as a remote, but I was originally proposing that you merge this PR into the ME-ICA/tedana@doc-tree branch (the new target branch for this PR) and make your changes there. |
Closes None.
Changes proposed in this pull request:
_comment
fields.